Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RoR forms project - Update CSRF token related sections #29165

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

RoR forms project - Update CSRF token related sections #29165

wants to merge 3 commits into from

Conversation

jg-k
Copy link

@jg-k jg-k commented Dec 8, 2024 •
edited
Loading

Because

Turbo is enabled by default from Rails 7 but tasks related to CSRF token in the first rails form project do not take into account default form behavior with Turbo.

This PR

  • Extends the tasks of the rails form project by getting the student to submit the manually created form without then with turbo enabled to provide a better understanding of which CSRF token is needed where.
  • Removes the paragraph explaining how to add a hidden input with a CSRF token from the lesson and relocates it to the project section. This avoids introducing too much details related to Turbo in the lesson, as it would extend beyond the core topic of the lesson. It is proposed to address the impact of Turbo more comprehensively separately.

Issue

Closes #28932

Additional Information

NA

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project curriculum contributing guide
  • The title of this PR follows the location of change: brief description of change format, e.g. Intro to HTML and CSS lesson: Fix link text
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If any lesson files are included in this PR, they have been previewed with the Markdown preview tool to ensure it is formatted correctly
  • If any lesson files are included in this PR, they follow the Layout Style Guide

@github-actions github-actions bot added the Content: Ruby on Rails Involves the Ruby on Rails course label Dec 8, 2024
@jg-k jg-k changed the title RoR forms project - Update CSRF related sections RoR forms project - Update CSRF token related sections Dec 8, 2024
@wise-king-sullyman wise-king-sullyman requested review from a team and rlmoser99 and removed request for a team December 10, 2024 01:57
rlmoser99
rlmoser99 requested changes Dec 12, 2024
View reviewed changes
Copy link
Member

@rlmoser99 rlmoser99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking the time to make these changes to these two lessons. There are a few small things that I think we need to change, so that the layout matches the rest of the lesson/curriculum. I left a few suggestions to try and illustrate why I am requesting a change, but I am open to other ideas as well.

ruby_on_rails/forms_and_authentication/project_forms.md Outdated
Comment on lines 37 to 39
1. CSRF Safety:
From Rails 7, Turbo is enabled by default in new apps. Turbo intercepts form submission and makes a partial XHR request instead of a standard HTTP request with full page reload. To get a better grasp of Rails protection against [cross-site request forgery](https://en.wikipedia.org/wiki/Cross-site_request_forgery), let's take a small detour and disable Turbo for this form by setting the data attribute `data-turbo=false`.
In the dev tools network tab, compare the request type with and without the `data-turbo=false` attribute to confirm it works as expected.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using our markdown preview tool, I don't really like having the "CSRF Safety:" and the last sentence on different lines look. I propose rewording slightly to putting them all on 1 line, something like this:

Suggested change
1. CSRF Safety:
From Rails 7, Turbo is enabled by default in new apps. Turbo intercepts form submission and makes a partial XHR request instead of a standard HTTP request with full page reload. To get a better grasp of Rails protection against [cross-site request forgery](https://en.wikipedia.org/wiki/Cross-site_request_forgery), let's take a small detour and disable Turbo for this form by setting the data attribute `data-turbo=false`.
In the dev tools network tab, compare the request type with and without the `data-turbo=false` attribute to confirm it works as expected.
1. For CSRF safety with Rails 7, Turbo is enabled by default in new apps. Turbo intercepts form submission and makes a partial XHR request instead of a standard HTTP request with full page reload. To get a better grasp of Rails protection against [cross-site request forgery](https://en.wikipedia.org/wiki/Cross-site_request_forgery), let's take a small detour and disable Turbo for this form by setting the data attribute `data-turbo=false`. Then, in the dev tools network tab, compare the request type with and without the `data-turbo=false` attribute to confirm it works as expected.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as proposed

ruby_on_rails/forms_and_authentication/form_basics.md Outdated
### Making forms into params

What about the other form inputs, the ones we actually care about?
### Railsifying your form - Making forms input into params
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of combining these two titles with a hyphen, I'd prefer to combine them together. Maybe something like this:

Suggested change
### Railsifying your form - Making forms input into params
### Railsifying your form by making the forms input into params

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as proposed

ruby_on_rails/forms_and_authentication/project_forms.md Outdated
Comment on lines 123 to 124
1. Check your inspector and your `application.html.erb` template. See a CSRF token that s always available?
Remove this one too from `application.html.erb`, and verify that the server hits back with a CSRF error.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo here: "that s always available?".

Also, I'm not a big fan of how this looks in the markdown preview. I would either put these two lines on the same line, or add an empty line in between (line lines 118-120).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated and put in a single paragraph.

ruby_on_rails/forms_and_authentication/project_forms.md Outdated
Comment on lines 118 to 121
1. Re-enable form submission with Turbo by removing the `data-turbo=false` attribute on the form tag, then also remove the hidden input with CSRF token tag and submit.

No more CSRF error!?!
The from is now submitted with Turbo, yet Rails still protects you by verifying a CSRF token. Where does this token comes from?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a fan of how these lines lay out in the markdown preview either, though I don't really have a great solution in mind. Maybe add an empty line between 120 and 121?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I have added an empty line to keep the "No more CSRF error" as a standalone observation that the reader should pause on. I did not put it on a separate numbered list item as it s not a new task.

@jg-k
Copy link
Author

jg-k commented Dec 13, 2024

Thanks for the review 🙏

@jg-k
Copy link
Author

jg-k commented Dec 20, 2024

@rlmoser99
please let me know if you have any further comments :-)

Or if you think the changes are not necessary and the PR should be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rlmoser99 rlmoser99 rlmoser99 requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Content: Ruby on Rails Involves the Ruby on Rails course
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rails: update first form project description to account for default form submission behavior with Turbo
2 participants
@jg-k @rlmoser99